Avoid needless copies reported by Coverity Scan - #13550
Merged
Merged
Conversation
Replaces a copy with a move where the source is not used again, and binds a reference instead of copying where a loop variable or local only reads the referent. No behavior change: every move source was checked to be dead after the move, and every reference was checked to outlive its use. Adds <utility> to four files that now name std::move but did not include it directly. Verified with a clean build (no new warnings) and the full unit test suite on Fedora, GCC 16.1.1.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is part of a larger Coverity Scan cleanup and focuses on eliminating unnecessary copies by switching to std::move where appropriate and binding references instead of copying values, with the stated intent of no behavior changes.
Changes:
- Replaces various local copies with moves when the source is not used again (e.g., push/insert into containers, assignments, parameter passing).
- Converts some range/loop and local variable copies to
const auto &to avoid copying read-only values. - Adds missing
<utility>includes in several files that now directly usestd::move.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tsutil/Metrics.cc | Removes misleading std::move on a const & parameter when pushing into the derived-metrics list. |
| src/tscore/runroot.cc | Uses std::move for assigned runroot paths and map values; adds <utility>. |
| src/tscore/Layout.cc | Uses moves when transferring temporary strings into path/prefix. |
| src/tscore/ArgParser.cc | Moves lookup_key into the stored option record to avoid an extra copy. |
| src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h | Moves per-item decoded structs into the response list. |
| src/traffic_ctl/CtrlCommands.cc | Moves plugin message params into the request object. |
| src/proxy/http/PreWarmManager.cc | Moves config/shared objects into newly built reconfiguration map entries. |
| src/proxy/HostStatus.cc | Moves per-host status objects into the output vector. |
| src/iocore/net/UnixNetAccept.cc | Moves per-accept ConnectionTracker::Group into the VC to avoid shared_ptr refcount churn. |
| src/iocore/net/SSLUtils.cc | Moves generated certificate/key path strings and name sets into containers/variables. |
| src/iocore/net/SSLNetVConnection.cc | Moves the shared session pointer into the connection; adds <utility>. |
| src/iocore/net/SSLCertLookup.cc | Avoids copying secret policy names by iterating with const &. |
| src/config/ssl_multicert.cc | Moves result/errata in early returns to avoid unnecessary vector copies. |
| src/api/InkAPI.cc | Avoids an extra YAML::Node copy in TSRPCHandlerDone by binding a reference. |
| plugins/traffic_dump/transaction_data.cc | Avoids copying the stored HTTP version by binding a const &. |
| plugins/traffic_dump/session_data.cc | Moves log filename into session data; adds <utility>. |
| plugins/origin_server_auth/origin_server_auth.cc | Moves region into the map entry to avoid a copy. |
| plugins/header_rewrite/operators.cc | Avoids copying parser arg/value strings when initializing run-plugin. |
| plugins/experimental/stek_share/stek_share.cc | Moves shared_ptr/nuraft pointers into stored state and initialization calls. |
| plugins/experimental/stek_share/state_manager.h | Moves newly created server config pointers into the saved config list; adds <utility>. |
| plugins/experimental/stek_share/state_machine.h | Moves snapshot context into the stored snapshot pointer. |
| plugins/experimental/stek_share/log_store.cc | Moves cloned / serialized nuraft objects into containers/slots. |
| plugins/experimental/rate_limit/txn_limiter.cc | Moves tag/prefix into metrics initialization; adds <utility>. |
| plugins/experimental/rate_limit/sni_selector.cc | Moves alias strings into addAlias to avoid a copy. |
| plugins/experimental/jax_fingerprint/ja4h/test.cc | Avoids copying map entries in iteration by using const &. |
| plugins/experimental/access_control/pattern.cc | Moves captured strings into result vectors to avoid copies. |
| plugins/experimental/access_control/config.cc | Moves parsed secret values into containers and logs from the stored container value. |
| plugins/esi/lib/EsiParser.cc | Moves newly created nodes into node lists; adds <utility>. |
| plugins/cachekey/configs.cc | Avoids copying parsed key types by iterating with const &. |
| plugins/cachekey/cachekey.cc | Moves constructed header strings into the capture set. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
TSRPCHandlerDone only reads the node, so casting to a const pointer and binding a const reference says that at the call site instead of handing out a mutable reference to a caller-owned node.
bryancall
marked this pull request as ready for review
August 17, 2026 17:27
cmcfarlen
approved these changes
Aug 24, 2026
cmcfarlen
left a comment
cmcfarlen
left a comment
Contributor
There was a problem hiding this comment.
Review
64/-57 across 30 files with no behavior change claimed. I spot-checked every site where a moved-from source could plausibly be reused, and the claim holds.
Verified
- The three
conn_track_groupmoves (your own flagged risk) are safe: declared inside thedo{}whilebody atUnixNetAccept.cc:128,:406,:528, moved at:138,:423,:591, no use afterward in the iteration. Had any been hoisted, inbound connection tracking would silently die after the first connection — good instinct to call it out. PreWarmManager.cc:927/:940—confandstats_idsare both loop/branch-local, safe.SSLUtils.cc:2234common_names = std::move(name_set)—name_setis declared at:2189inside the per-cert loop; thefirst_passbranch doesn't touch it afterward and the next iteration redeclares it. Safe.runroot.cc— everyrunroot_file = std::move(path)is followed byreturn, orpathis reassigned before the next read. Safe.Layout.cc,ArgParser.cc,sni_selector.cc,txn_limiter.cc— all sinks take by value (addAlias(std::string),initializeMetrics(uint, std::string, std::string)), sources dead after. Correct move-into-sink.operators.cc:1272const auto &plugin_name = p.get_arg()—get_arg()returnsstd::string &andget_value()returnsconst std::string &, both to members, andpisn't mutated afterward. No temporary, genuine copy elision.access_control/config.cc— the debug lines were correctly re-pointed tomap[key]/vector.back()after the move. Logically right, though as you note nothing compiles them.Metrics.cc:257—push_back(std::move(m))→push_back(m)on aconst DerivedMetric &: the oldstd::movewas a silent no-op (a const lvalue binds to the copy constructor), so this is a real readability fix with identical semantics. Good catch.
One correction to the reasoning in the description
The justification for skipping the NextHop cases is stated as: const auto &x = n["scheme"].Scalar() "binds a reference into a temporary that dies at the end of the statement." The conclusion is right but the reason isn't — const auto &x = <temporary> does lifetime-extend. The actual hazard is that n["scheme"] is an intermediate temporary Node and .Scalar() returns a reference into it; extension applies only to the final temporary, not the intermediate. Worth stating precisely, since the imprecise version would also argue against conversions that are in fact safe.
Out of scope, but noticed while verifying
HostStatus.cc:307— this fixeshosts.push_back(h)but leaves the loop headerfor (std::pair<std::string, HostStatRec *> hsts : hosts_statuses), which copies astd::stringper iteration. Same file, same class of finding;auto const &would complete it.rate_limit/limiter.h:228—initializeMetrics(RATE_LIMITER_TYPE_SNI, prefix, tag)against the signatureinitializeMetrics(uint type, std::string tag, std::string prefix). The arguments look transposed, and theDbgimmediately above prints them in(prefix, tag)order, which suggests that's how the author was thinking. Pre-existing and not in this diff —txn_limiter.cc, which this PR does touch, calls it correctly — but if it's real, a user-configured SNI metric prefix and tag land in each other's slots. Worth a separate look.
Approving. The two out-of-scope items are follow-ups, not blockers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 1 of 3 splitting a Coverity Scan cleanup into independently reviewable pieces. This one is deliberately the boring part: no behavior change anywhere.
What this does
std::movewhere the source is never used again (42 sites).const auto &/auto const &instead of copying where a loop variable or local only reads the referent (7 sites).<utility>to four files that namestd::movewithout including it directly.How it was checked
Every move source was traced to the end of its scope to confirm it is not read after the move. The three
enable_inbound_connection_tracking(std::move(conn_track_group))sites are worth a second look if you want a spot check:conn_track_groupis declared inside each accept loop body, so no iteration inherits a moved-from group. A shared declaration there would have silently disabled inbound connection tracking after the first connection.Every reference conversion was checked to make sure it binds to something that outlives the use, not to a temporary.
Reports deliberately not acted on
Coverity flags five
autocopies in the next-hop YAML parsers (NextHopSelectionStrategy.cc,NextHopConsistentHash.cc). Those are false positives and are left alone: the node accessors return by value, soconst auto &x = n["scheme"].Scalar()binds a reference into a temporary that dies at the end of the statement. GCC's-Wdangling-referenceconfirms it.ConfigContextparameters reported as oversized are also left alone. They are by value by design, because the reload handler signature requires it and the handlers mutate the context.Verification
Clean build with no new warnings and the full unit test suite passing (137/137) on Fedora, GCC 16.1.1.
Getting every modified file actually compiled took three extra options, which is worth stating precisely rather than claiming full coverage:
uri_signingneeds cjose,stek_shareneeds nuraft, andjax_fingerprintdefaults to off. With-DENABLE_URI_SIGNING=ON -DENABLE_STEK_SHARE=ON -DENABLE_JAX_FINGERPRINT=ONall of them build and their objects appear in the graph.access_controlchanges sit behind#ifdef ACCESS_CONTROL_LOG_SECRETS, which no build here defines, so those two lines are reviewed but not compiled.Draft while CI runs.